Skip to content

chore: Use published router-bridge#1613

Merged
o0Ignition0o merged 6 commits intomainfrom
igni/published_bridge
Aug 26, 2022
Merged

chore: Use published router-bridge#1613
o0Ignition0o merged 6 commits intomainfrom
igni/published_bridge

Conversation

@o0Ignition0o
Copy link
Contributor

part of #491

We have published the router-bridge to crates.io, which removes the need for router developers to have nodejs installed.

part of #491

We have published the `router-bridge` to crates.io, which removes the need for router developers to have nodejs installed.
Remove nodejs from the CI steps.
@o0Ignition0o o0Ignition0o marked this pull request as ready for review August 25, 2022 15:57
@o0Ignition0o o0Ignition0o requested review from Geal, SimonSapin, abernix and garypen and removed request for abernix and garypen August 25, 2022 15:57
name: cargo check workspace
command: |
set -e -o pipefail
cargo check --locked
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this redundant with the build that's necessary for running tests?

curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.38.0/install.sh | bash
echo '. ~/.nvm/nvm.sh' >> $BASH_ENV
- run:
name: Install desired Node.js version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice to be able to remove all this!

@garypen
Copy link
Contributor

garypen commented Aug 26, 2022

part of #491

We have published the router-bridge to crates.io, which removes the need for router developers to have nodejs installed.

Does this mean we can get rid of the ASDF stuff in our repo as well now?

@o0Ignition0o
Copy link
Contributor Author

Does this mean we can get rid of the ASDF stuff in our repo as well now?
oh we could! let s try!

Copy link
Contributor

@garypen garypen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best PR ever! :)

@o0Ignition0o o0Ignition0o merged commit cb345d3 into main Aug 26, 2022
@o0Ignition0o o0Ignition0o deleted the igni/published_bridge branch August 26, 2022 09:05
@abernix abernix mentioned this pull request Aug 29, 2022
@abernix abernix added this to the v1.0.0-alpha.0 milestone Aug 29, 2022
@@ -1,5 +0,0 @@
# If updating this, you *must* also change the channel in rust-toolchain.toml
# renovate-automation: rustc version
rust 1.61.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@o0Ignition0o Curious — this PR was intended to remove the dependency on Node.js, which was indeed great, but I'm curious why you chose to remove rust from .tool-versions as well? Would we consider adding it back? Do we think it's unnecessary?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know .tool-versions is only used by ASDF. In my experience folks writing Rust regularly have $(which cargo) pointing to rustup, not ASDF. With this PR, ASDF is not needed at all to contribute to the Router.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants